Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SpriteFramesEditor Minor fixes #48984

Merged

Conversation

kleonc
Copy link
Member

@kleonc kleonc commented May 22, 2021

To match minor fixes from #48977.

@kleonc
Copy link
Member Author

kleonc commented May 22, 2021

@akien-mga Please don't merge yet.

I wanted to ask if using MAX(1.0, EDSCALE) instead of MAX(1.0f, EDSCALE) is intentional, for example here:
https://github.com/godotengine/godot/blob/88942a6637a9e460c715e3299758d4b85915aaac/editor/plugins/sprite_frames_editor_plugin.cpp#L1237-L1244
But now when I think of it, it probably doesn't really make a difference. 😄 And if it does, probably it's like this in way more places than just here.
So this should be fine to merge, sorry for stopping.

@kleonc kleonc requested a review from akien-mga May 22, 2021 23:29
@aaronfranke
Copy link
Member

aaronfranke commented May 23, 2021

@kleonc Indeed that would make sense. EDSCALE returns a float, and these fields are float, so it's better to use it consistently rather than casting to double and back to float. I checked if there are other places around the engine to fix, this is the only file with the exact text MAX(1.0, EDSCALE), so I think it's worth making that change in this PR (make them MAX(1.0f, EDSCALE)).

EDIT: Except thumbnail_default_size is an integer, so that one should be MAX(1, EDSCALE)

@kleonc kleonc force-pushed the sprite_frames_editor-minor-fixes branch from 88942a6 to 07d346d Compare May 23, 2021 09:00
@kleonc
Copy link
Member Author

kleonc commented May 23, 2021

@aaronfranke Ok, I've changed those.

@akien-mga akien-mga merged commit f096a58 into godotengine:master May 23, 2021
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants